Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a visitor for TypeFoldables and use it to fix cleanup issue #20298 #30317

Merged
merged 3 commits into from
Jan 7, 2016

Conversation

jseyfried
Copy link
Contributor

TypeFoldables can currently be visited inefficiently with an identity folder that is run only for its side effects. This creates a more efficient visitor for TypeFoldables and uses it to implement RegionEscape and HasProjectionTypes, fixing cleanup issue #20298.
This is a pure refactoring.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@jseyfried jseyfried force-pushed the refactor_type_folder branch from 7d4c2fd to 5913159 Compare December 11, 2015 03:30
@brson
Copy link
Contributor

brson commented Dec 14, 2015

r? @nikomatsakis

@@ -751,6 +658,18 @@ impl<'tcx> TypeFoldable<'tcx> for ty::Predicate<'tcx> {
ty::Predicate::ObjectSafe(trait_def_id),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like maybe we should have fold_subitems_with be unimplemented! on such cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikomatsakis My justification for having fold_subitems_with default to fold_with was that for types without corresponding methods in TypeVisitor, folding the subitems of the type is the same as folding the type.

On second though, I think it would nicest to have fold_with default to fold_subitems_with so that structural implementations always define fold_subitem_with to call fold_with on subitems and only define fold_with to call a method in TypeFolder when there is a corresponding method.

Did you want fold_subitems_with to be unimplemented! to reflect that users should only be calling fold_subitems_with when it is different from fold_with? If so, I think it would be better to enforce that at the type level by having a trait like TypeFolderOverridable define fold_subitems_with, or by using the original super_* setup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jseyfried

Did you want fold_subitems_with to be unimplemented! to reflect that users should only be calling fold_subitems_with when it is different from fold_with?

I think my reasoning was that outside users should always call fold_with, and the only reason to have fold_subitems_with was to use from the visitor method itself. I'm trying to think if there would be a correct scenario for calling fold_subitems_with on one of these cases where we haven't allowed for overriding in the visitor.

I think I agree with this comment though:

On second though, I think it would nicest to have fold_with default to fold_subitems_with so that structural implementations always define fold_subitem_with

...in the MIR visitor, I actually used the name super_fold_with, which I personally find more intuitive than fold_subitems_with. It also makes it clear that this is not something you normally want to call. Otherwise my setup was just as you described:

https://github.com/rust-lang/rust/blob/master/src/librustc/mir/visit.rs#L21-L23

Would you mind refactoring to use the super naming scheme and to reverse the defaults?

@jseyfried jseyfried force-pushed the refactor_type_folder branch 2 times, most recently from 9bb241b to 14c8af1 Compare December 16, 2015 01:41
@jseyfried
Copy link
Contributor Author

I rebased since a super_* had been added upstream and then added a new commit.

@jseyfried jseyfried force-pushed the refactor_type_folder branch 2 times, most recently from d99cc6c to aff9a33 Compare December 16, 2015 06:20
@bors
Copy link
Contributor

bors commented Dec 17, 2015

☔ The latest upstream changes (presumably #30325) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried jseyfried force-pushed the refactor_type_folder branch 3 times, most recently from 5d9a165 to 909b407 Compare December 18, 2015 11:54
@bors
Copy link
Contributor

bors commented Dec 18, 2015

☔ The latest upstream changes (presumably #30389) made this pull request unmergeable. Please resolve the merge conflicts.

@brson
Copy link
Contributor

brson commented Dec 28, 2015

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned brson Dec 28, 2015
@nikomatsakis
Copy link
Contributor

@jseyfried any thoughts on this question:

Would you mind refactoring to use the super naming scheme and to reverse the defaults?

I still think that'd be somewhat more clear overall. But this is a nice PR, I'd hate to see it bitrot away.

@jseyfried jseyfried force-pushed the refactor_type_folder branch 2 times, most recently from dedfed1 to 2a2bae7 Compare January 6, 2016 02:03
@bors
Copy link
Contributor

bors commented Jan 6, 2016

☔ The latest upstream changes (presumably #30532) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

Argh! @jseyfried I did not see that you had pushed those new commits! (Leaving a comment can help in that regard, since it generates an e-mail.) They look good though, r+ after rebase.

@jseyfried jseyfried force-pushed the refactor_type_folder branch from 2a2bae7 to 6327563 Compare January 7, 2016 00:44
@jseyfried
Copy link
Contributor Author

@nikomatsakis Just rebased. Sorry for the delay on this PR, I was off grid for a while over the holidays.

I hadn't commented yet since I wanted to check that Travis CI was passing first -- the build was failing less than a minute in for a while last night for some reason but it looks like that got resolved.

I had chosen fold_subitems_with because I thought it made the semantics clearer, but I can see how super_fold_with better communicates the intended use and is more consistent.

@nikomatsakis
Copy link
Contributor

@jseyfried thanks for updating it!

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 7, 2016

📌 Commit 6327563 has been approved by nikomatsakis

bors added a commit that referenced this pull request Jan 7, 2016
`TypeFoldable`s can currently be visited inefficiently with an identity folder that is run only for its side effects. This creates a more efficient visitor for `TypeFoldable`s and uses it to implement `RegionEscape` and `HasProjectionTypes`, fixing cleanup issue #20298.
This is a pure refactoring.
@bors
Copy link
Contributor

bors commented Jan 7, 2016

⌛ Testing commit 6327563 with merge 1e83503...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants